Resolve OpenSSL 4.0 build issues - #13476
Conversation
|
Did you consider the |
|
[approve ci] |
|
@JosiahWI Thanks for the reminder. The use of |
|
I think |
|
Oh, I see. The |
There was a problem hiding this comment.
Pull request overview
This PR continues the effort to keep ATS building across OpenSSL versions (OpenSSL 1.1.1 through 4.0+/constified APIs and BoringSSL) by tightening const-correctness around X509/ASN1 access and updating code to use supported accessor functions instead of direct structure field access.
Changes:
- Replace direct
ASN1_STRING/ASN1_BIT_STRINGfield access (->data,->length,->type) with accessor APIs likeASN1_STRING_get0_data(),ASN1_STRING_length(), andASN1_STRING_type(). - Update several helper signatures and local variables to
constwhere the OpenSSL APIs now expose const data (e.g.,X509_NAME,X509_NAME_ENTRY,ASN1_STRING). - Adjust certificate subject-name manipulation in
certifierto operate on a duplicatedX509_NAMEand set it back onto the certificate.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tscore/X509HostnameValidator.cc | Uses ASN1 accessor APIs for hostname checks and adjusts CN parsing types. |
| src/iocore/net/SSLUtils.cc | Makes asn1_strdup() accept const ASN1_STRING* and propagates const correctness through subject CN extraction. |
| src/iocore/net/SSLNetVConnection.cc | Updates debug helper to accept const X509_NAME*. |
| src/iocore/net/OCSPStapling.cc | Replaces ASN1_BIT_STRING direct field access with ASN1 accessor APIs for digesting and const cleanup. |
| src/api/InkAPI.cc | Uses const-correct X509_NAME_ENTRY / ASN1_STRING pointers when extracting CN during cert update. |
| plugins/experimental/txn_box/plugin/src/ts_util.cc | Makes ssl_value_for() accept const X509_NAME* and uses a const_cast for OpenSSL 1.1.1 compatibility at one call site. |
| plugins/experimental/sslheaders/expand.cc | Makes subject/issuer names const and replaces signature byte access with ASN1 accessor APIs. |
| plugins/experimental/cert_reporting_tool/cert_reporting_tool.cc | Treats subject name as const X509_NAME*. |
| plugins/certifier/certifier.cc | Duplicates and sets the subject name before adding CN to avoid mutating internal OpenSSL-owned structures directly. |
|
I had independently worked up a fix for this (#13482) before seeing this PR — standing that down in favor of yours. Handing over what I found in case it's useful, since some of it isn't covered here yet. The diagnosis and the three I built this branch in a Fedora 45 / OpenSSL 4.0.1 container (
Specifically:
Two notes on approach, take or leave:
Diffs for all of the above are on |
OpenSSL 1.1.1 declares the first argument as non-const while newer releases declare it const, so building against different versions failed depending on which signature was in effect. Add a const_cast at each call site to keep the const-qualified variables introduced by this PR buildable against both, matching the existing precedent in OCSPStapling.cc.
X509_get_subject_name and X509_NAME_get_index_by_NID have their argument constness changed together across OpenSSL versions, so a local variable declared with auto tracks whatever type is correct for the OpenSSL version in use, without a cast. This applies only to the two purely local variables; ts_util.cc ssl_value_for keeps its const_cast since its parameter type is shared across multiple callers.
X509HostnameValidator.cc cast ASN1_STRING_get0_data return value to non-const before an ats_strndup call that only wants const char *; drop the const instead of adding it back needlessly. certifier.cc leaked the duplicated X509_NAME on the X509_NAME_add_entry_by_txt failure path, and never checked X509_NAME_dup for allocation failure.
CertBase::X509Value took hardcoded function pointer types for X509_get_subject_name, X509_get_issuer_name, X509_getm_notBefore, and X509_getm_notAfter, but those accessors change constness in different directions across OpenSSL versions, so no single hardcoded signature builds everywhere. Deduce the parameter type from the actual accessor via decltype instead. Signature::_load and _write_ip_address also read ASN1_STRING fields directly, which breaks once the struct is opaque; switch to the accessor functions used elsewhere in this codebase.
get_x509_name_string only reads through the name via X509_NAME_print_ex, so accept a const X509_NAME * to match callers that pass X509_get_subject_name/X509_get_issuer_name results directly. get_x509_signature_string read the ASN1_STRING struct fields directly, which breaks once the struct is opaque; use ASN1_STRING_get0_data/ASN1_STRING_length instead.
make_cert_and_key mutated the X509_NAME returned by X509_get_subject_name in place, which stops compiling once that accessor can return const, and was already fragile since the returned name is only a view into the certificate internal state. Duplicate it, add the CN to the duplicate, and set it back as both subject and issuer name since this is a self-signed certificate.
client_context_dump.cc, verify_cert.cc, and ssl_client_verify_test.cc all held X509_get_subject_name/X509_NAME_get_entry/ X509_NAME_ENTRY_get_data results in hardcoded non-const locals or parameters, which stops compiling once those accessors return const. Switch to auto for the local variables and const for the debug_certificate parameter, matching the read-only usage in each case.
ssl_value_for is shared by four callers, each already deducing its X509_NAME pointer type with auto from X509_get_subject_name or X509_get_issuer_name, so unlike the other three call sites fixed earlier in this series, a single hardcoded parameter type cannot track the underlying accessor across OpenSSL versions. Templating the parameter on the callers deduced type removes the cast entirely.
Bare decltype(&X509_get_subject_name) in a parameter list reads poorly at each of the four call sites; name each getter type once via using so the declarations and out-of-line definitions just say what kind of accessor they take.
certifier.cc freed the duplicated X509_NAME manually on every path; switch to a scoped_X509_NAME unique_ptr matching the file existing scoped_X509/scoped_EVP_PKEY/scoped_SSL_CTX aliases so no path can forget to free it. ts_util.cc templated ssl_value_for just to defer the parameter type to the caller; decltype(X509_get_subject_name(nullptr)) deduces the same pointer type directly without turning it into a template. Also drop the last C-style cast this series introduced in X509HostnameValidator.cc in favor of reinterpret_cast.
d31a219 to
01b9317
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
plugins/certifier/certifier.cc:411
X509_NAME_add_entry_by_txt()expects aconst unsigned char *, but this code uses a non-const cast(unsigned char *)commonName.c_str(). Avoid casting away constness here (and avoid C-style casts) to keep the code const-correct across OpenSSL variants.
if (X509_NAME_add_entry_by_txt(n.get(), "CN", MBSTRING_ASC, (unsigned char *)commonName.c_str(), -1, -1, 0) != 1) {
plugins/experimental/txn_box/plugin/src/ts_util.cc:1119
- The PR description says the OpenSSL 1.1.1 vs 3.x+
X509_NAME_get_index_by_NID()const-mismatch is handled by addingconst_cast<X509_NAME*>(...)at the affected call sites, but this change instead introduces adecltype(X509_get_subject_name(nullptr))alias and does not add any suchconst_cast(and none exist in-tree). Please update the PR description to match the implemented approach, or adjust the code to match the documented fix.
using X509_NAME_ptr = decltype(X509_get_subject_name(nullptr));
TextView
ssl_value_for(X509_NAME_ptr name, int nid)
{
if (int loc = X509_NAME_get_index_by_NID(name, nid, -1); loc >= 0) {
| unsigned char *astr; | ||
| str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); | ||
| // Convert to UTF-8 | ||
| astrlen = ASN1_STRING_to_UTF8(&astr, str); |
There was a problem hiding this comment.
Confirmed real, but pre-existing on master since the function's original addition in 2015, unrelated to this PR's OpenSSL 4 changes. Filed as #13483.
| const int pos = X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName, -1); | ||
| const X509_NAME_ENTRY *common_name = X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos); | ||
| const ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); | ||
| char *common_name_str = reinterpret_cast<char *>(const_cast<unsigned char *>(ASN1_STRING_get0_data(common_name_asn1))); | ||
| if (ASN1_STRING_length(common_name_asn1) != static_cast<int>(strlen(common_name_str))) { |
There was a problem hiding this comment.
Confirmed real, but pre-existing on master since 2024, unrelated to this PR's OpenSSL 4 changes. Filed as #13484.
|
[approve ci autest 2] |
|
I've updated the change and addressed all the comments. New code does not use const_cast at all now. I did see |
|
@jeredfloyd I added changes on top of your commits to cover the entire source tree. I'd appreciate it if you could test this change for Fedora 45. |
There was a problem hiding this comment.
This looks great to me. Will wait for @jeredfloyd to check before approving. I have one question about the certifier logic.
jeredfloyd
left a comment
There was a problem hiding this comment.
Looks good to me, and confirmed this builds on Fedora 45 and EPEL 10. (I'm old and prefer explicit types over inferred wherever possible, and this looks like a reasonable balance.)
Summary
Continues #13440 ("Resolve OpenSSL 4.0 build issues"), fixing the remaining CI failures on Ubuntu 20.04 and FreeBSD 13.1 (OpenSSL 1.1.1) flagged in review.
X509_NAME_get_index_by_NID()takes a non-constX509_NAME *on OpenSSL 1.1.1 but aconst X509_NAME *on OpenSSL 3.0+ and post-constify OpenSSL/BoringSSL. The original PR widened severalX509_NAME-typed variables toconstto build against the newer, constified signature, which broke the older signature at three call sites:src/tscore/X509HostnameValidator.cc—validate_hostname()src/iocore/net/SSLUtils.cc—SSLMultiCertConfigLoader::load_certs_and_cross_reference_names()plugins/experimental/txn_box/plugin/src/ts_util.cc—ssl_value_for()Each call site now does
const_cast<X509_NAME *>(...), matching the existing precedent inOCSPStapling.ccfor the same kind of OpenSSL-version const mismatch.Test plan
tscore,inknet, andtxn_boxtargets locally against BoringSSL — all compile cleancmake --build build -t format— no additional changes